Fix message if no files found #3650

craigtaub · 2019-01-03T18:41:37Z

Description of the Change

Previously would print a warning for each missing file/dir, then an error if no valid files were found.
Now prints slimmer warnings and errors only if no tests found.
Tests cover both scenarios.

Alternate approach

Print an error for each missing file/dir (and no final error). Still exit if no valid files were found.
Nasty as flags everything as an error even if there is a passing test

Scenarios:

Nothing given

Command: mocha
New output:

Error: Cannot find any files matching pattern "test/"

Previous output:

Warning: cannot find any files matching pattern "test/": 'test/'
✖ No test files found

Difference = No warning and slimmer message

Nothing found

Command: mocha -R json-stream --no-config "test/integration/fixtures/glob/**/*-none.js"
New output:

Error: Cannot find any files matching pattern "test/integration/fixtures/glob/**/*-none.js"

Previous output:

Warning: cannot find any files matching pattern "test/integration/fixtures/glob/**/*-none.js": 'test/integration/fixtures/glob/**/*-none.js'
✖ No test files found

Difference = No warning and slimmer message

1 found but 1 not found:

Command: mocha -R json-stream --no-config "test/integration/fixtures/glob/**/*.js" "test/integration/fixtures/glob/**/*-none.js"
New output:

Warning: Cannot find any files matching pattern "test/integration/fixtures/glob/**/*-none.js"
["start",{"total":2}]
["pass",{"title":"should find this test","fullTitle":"globbing test should find this test","duration":0,"currentRetry":0}]
["pass",{"title":"should find this test","fullTitle":"globbing test should find this test","duration":0,"currentRetry":0}]
["end",{"suites":2,"tests":2,"passes":2,"pending":0,"failures":0,"start":"2019-01-03T19:03:34.591Z","end":"2019-01-03T19:03:34.593Z","duration":2}]

Previous output:

Warning: cannot find any files matching pattern "test/integration/fixtures/glob/**/*-none.js": 'test/integration/fixtures/glob/**/*-none.js'
["start",{"total":2}]
["pass",{"title":"should find this test","fullTitle":"globbing test should find this test","duration":0,"currentRetry":0}]
["pass",{"title":"should find this test","fullTitle":"globbing test should find this test","duration":0,"currentRetry":0}]
["end",{"suites":2,"tests":2,"passes":2,"pending":0,"failures":0,"start":"2019-01-03T17:53:56.843Z","end":"2019-01-03T17:53:56.845Z","duration":2}]

Difference = slimmer warning

1 found but 2 not found:

Command: mocha -R json-stream --no-config "test/integration/fixtures/glob/**/*.js" "test/integration/fixtures/glob/**/*-none.js" "test/integration/fixtures/glob/**/*-none-again.js"
New output:

Warning: Cannot find any files matching pattern "test/integration/fixtures/glob/**/*-none.js"
Warning: Cannot find any files matching pattern "test/integration/fixtures/glob/**/*-none-again.js"
["start",{"total":2}]
["pass",{"title":"should find this test","fullTitle":"globbing test should find this test","duration":0,"currentRetry":0}]
["pass",{"title":"should find this test","fullTitle":"globbing test should find this test","duration":0,"currentRetry":0}]
["end",{"suites":2,"tests":2,"passes":2,"pending":0,"failures":0,"start":"2019-01-03T19:03:55.208Z","end":"2019-01-03T19:03:55.210Z","duration":2}]

Previous output:

Warning: cannot find any files matching pattern "test/integration/fixtures/glob/**/*-none.js": 'test/integration/fixtures/glob/**/*-none.js'
Warning: cannot find any files matching pattern "test/integration/fixtures/glob/**/*-none-again.js": 'test/integration/fixtures/glob/**/*-none-again.js'
["start",{"total":2}]
["pass",{"title":"should find this test","fullTitle":"globbing test should find this test","duration":0,"currentRetry":0}]
["pass",{"title":"should find this test","fullTitle":"globbing test should find this test","duration":0,"currentRetry":0}]
["end",{"suites":2,"tests":2,"passes":2,"pending":0,"failures":0,"start":"2019-01-03T17:54:34.393Z","end":"2019-01-03T17:54:34.395Z","duration":2}]

Difference = Simmer warnings

2 not found:

Command: mocha -R json-stream --no-config "test/integration/fixtures/glob/**/*-none.js" "test/integration/fixtures/glob/**/*-none-again.js"
New output:

Error: Cannot find any files matching pattern "test/integration/fixtures/glob/**/*-none.js"
Error: Cannot find any files matching pattern "test/integration/fixtures/glob/**/*-none-again.js"

Previous output:

Warning: cannot find any files matching pattern "test/integration/fixtures/glob/**/*-none.js": 'test/integration/fixtures/glob/**/*-none.js'
Warning: cannot find any files matching pattern "test/integration/fixtures/glob/**/*-none-again.js": 'test/integration/fixtures/glob/**/*-none-again.js'
✖ No test files found

Difference = Warnings swapped for errors and no extra error.

1 found:

Command: mocha -R json-stream --no-config "test/integration/fixtures/glob/**/*.js"
New output:

["start",{"total":2}]
["pass",{"title":"should find this test","fullTitle":"globbing test should find this test","duration":0,"currentRetry":0}]
["pass",{"title":"should find this test","fullTitle":"globbing test should find this test","duration":0,"currentRetry":0}]
["end",{"suites":2,"tests":2,"passes":2,"pending":0,"failures":0,"start":"2019-01-03T17:01:17.934Z","end":"2019-01-03T17:01:17.938Z","duration":4}]

Previous output:

["start",{"total":2}]
["pass",{"title":"should find this test","fullTitle":"globbing test should find this test","duration":0,"currentRetry":0}]
["pass",{"title":"should find this test","fullTitle":"globbing test should find this test","duration":0,"currentRetry":0}]
["end",{"suites":2,"tests":2,"passes":2,"pending":0,"failures":0,"start":"2019-01-03T17:55:06.996Z","end":"2019-01-03T17:55:06.999Z","duration":3}]

Difference = nothing

Applicable issues

Fixes #3646
(semver-patch)

coveralls · 2019-01-03T19:00:40Z

Coverage increased (+0.01%) to 90.739% when pulling 475d81c on craigtaub/fixNoisyMessage into 1f325e7 on master.

boneskull

LGTM, no major issues. you can change some stuff I've commented on if you want 👍

lib/cli/run-helpers.js

boneskull · 2019-01-03T19:32:29Z

test/integration/glob.spec.js

            results.stderr,
            'to contain',
-            'cannot find any files matching pattern "./*-none.js"'
+            '✖ Cannot find any files matching pattern "./*-none.js"'


can you pull in the symbol from log-symbols? this may fail on windows

Or just leave it and the pattern out...

'Cannot find any files matching pattern'

I like idea including symbol as could erroneously fall into warning block if just pattern matching the message.

Continuing with that thought, confusing to use the same message as both error and warning.

if (!files.length) { console.error(ansi.red('Error: No test files found...')); process.exit(1); } else { messages.forEach(message => { console.warn(ansi.yellow(`Warning: ${message}`)); }); }

Hmm I actually think listing the errors is better, from the users perspective it's more helpful and easier to see the mistake that way. I've used it myself during his pr.

So if user gives 20 globs and none exist, one message per is somehow more informative than just saying "none found"? I disagree.
For warnings, fine -- but not when none exist.

As @craigtaub stated, it was useful information, so if some subset of the paths are not found, it seems reasonable to list them (but not if all of them are not found).

FWIW, a user can avoid this problem altogether by using shell expansion, but that's not portable.

If a subset don't exist, those would be warnings -- stated I had no problem with that.
Just the files.length === 0 error case.

Ok i think its clear you both feel the single error message is better than multiple error messages. For debugging purposes I found the multiple one better, the initial idea of using an error was to indicate nothing did run, but perhaps its not clear enough.

lib/cli/run-helpers.js

plroebuck · 2019-01-03T21:12:46Z

test/integration/glob.spec.js

            results.stderr,
            'to contain',
-            'cannot find any files matching pattern'
+            '✖ Cannot find any files matching pattern "./*-none.js"'


plroebuck · 2019-01-03T21:13:05Z

test/integration/glob.spec.js

              results.stderr,
              'to contain',
-              'cannot find any files matching pattern'
+              '✖ Cannot find any files matching pattern "./**/*-none.js"'


plroebuck · 2019-01-04T18:44:19Z

lib/cli/run-helpers.js

+    // print messages as an error
+    errors.forEach(message => {
+      console.error(ansi.red(`Error: ${message}`));
+    });


This should have simply said:

console.error(ansi.red('Error: No test files found'));

Nothing is gained by outputting multiple error messages, and now we have both warnings and errors with the same message.

They don't show at the same time though (warnings and errors). I mentioned in my comment earlier I felt giving multiple error messages was helpful in those scenarios (in that I found it useful myself).

I'm a bit confused about when we are going to show errors vs warnings...

Wasn't the name of the issue "too much noise if no files found"? Retry one of your tests with lots of nonexistent globs...

I just didn't want to see a warning about it (which duplicated the filenames) and then another error. If that's fixed, then great

Check the description I tried to give full details on what shows when.

I already understand what would be displayed when. Disagreed about what should show for the error case (nothing matched) -- an "n not found" scenario with this code outputs n error messages instead of one. Worse, it uses the exact same message text as the warning case.

plroebuck · 2019-01-04T18:46:34Z

test/integration/glob.spec.js

            results.stderr,
            'to contain',
-            'cannot find any files matching pattern "./*-none.js"'
+            'Error: Cannot find any files matching pattern "./*-none.js"'


test pattern should not be part of check for maintainability. WET due to filename.

why was this change request ignored?

Wasn't on purpose, I thought there was nothing outstanding so I merged it.
Have made a new PR continuing (referenced) the work, I think it's more practical to converse there.

plroebuck · 2019-01-04T18:46:46Z

test/integration/glob.spec.js

            results.stderr,
            'to contain',
-            'cannot find any files matching pattern'
+            'Error: Cannot find any files matching pattern "./*-none.js"'


test pattern (filename) should not be part of check for maintainability.

plroebuck · 2019-01-04T18:47:00Z

test/integration/glob.spec.js

              results.stderr,
              'to contain',
-              'cannot find any files matching pattern'
+              'Error: Cannot find any files matching pattern "./**/*-none.js"'


test pattern (filename) should not be part of check for maintainability.

plroebuck · 2019-01-04T18:52:36Z

lib/cli/run-helpers.js

  spec = []
 } = {}) => {
  let files = [];
+  const errors = [];


More specific about what you're storing since you're not storing the Error instance that was thrown.

const messages = [];

craigtaub added 5 commits January 3, 2019 16:30

error and exit, dont warn

db722d0

return after error dont exit yet

96c3b15

remove superflous message

c4651cb

update glob tests

54bfedd

update message

964f363

craigtaub added area: reporters involving a specific reporter semver-minor implementation requires increase of "minor" version number; "features" pr-needs-work labels Jan 3, 2019

use errors only if no tests found

afc429e

craigtaub removed the pr-needs-work label Jan 3, 2019

craigtaub requested a review from boneskull January 3, 2019 19:11

boneskull approved these changes Jan 3, 2019

View reviewed changes

plroebuck reviewed Jan 3, 2019

View reviewed changes

lib/cli/run-helpers.js Outdated Show resolved Hide resolved

plroebuck reviewed Jan 3, 2019

View reviewed changes

dont use error symbol.

475d81c

craigtaub added this to the v6.0.0 milestone Jan 4, 2019

craigtaub merged commit fc0507e into master Jan 4, 2019

craigtaub deleted the craigtaub/fixNoisyMessage branch January 4, 2019 08:11

plroebuck reviewed Jan 4, 2019

View reviewed changes

craigtaub mentioned this pull request Jan 4, 2019

Fix messages for file lookup issues - error scenario #3654

Merged

plroebuck assigned craigtaub Jan 5, 2019

boneskull modified the milestones: v6.0.0, v6.0.0-2 Feb 11, 2019

noomorph mentioned this pull request Apr 17, 2019

feat: undeprecate "specs" in package.json wix/Detox#1290

Merged

This was referenced Oct 26, 2019

[Snyk] Upgrade mocha from 4.1.0 to 6.2.1 ajesse11x/marketplace#5

Open

[Snyk] Upgrade mocha from 2.5.3 to 6.2.1 ajesse11x/kompose#1

Open

Uh oh!

Fix message if no files found #3650

Fix message if no files found #3650

Uh oh!

Conversation

craigtaub commented Jan 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of the Change

Alternate approach

Scenarios:

Nothing given

Nothing found

1 found but 1 not found:

1 found but 2 not found:

2 not found:

1 found:

Applicable issues

Uh oh!

coveralls commented Jan 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

boneskull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

craigtaub Jan 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plroebuck Jan 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plroebuck Jan 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plroebuck Jan 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

craigtaub Jan 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plroebuck Jan 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

craigtaub Jan 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plroebuck Jan 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plroebuck Jan 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

craigtaub commented Jan 3, 2019 •

edited

Loading

coveralls commented Jan 3, 2019 •

edited

Loading

craigtaub Jan 3, 2019 •

edited

Loading

plroebuck Jan 3, 2019 •

edited

Loading

plroebuck Jan 4, 2019 •

edited

Loading

plroebuck Jan 4, 2019 •

edited

Loading

craigtaub Jan 4, 2019 •

edited

Loading

plroebuck Jan 4, 2019 •

edited

Loading

craigtaub Jan 4, 2019 •

edited

Loading

plroebuck Jan 5, 2019 •

edited

Loading

plroebuck Jan 4, 2019 •

edited

Loading

craigtaub Jan 5, 2019 •

edited

Loading

plroebuck Jan 4, 2019 •

edited

Loading